Fix thread-safety bug in geom.c#75
Conversation
This changes the semantics. But it's probably a good idea anyway since |
This is potentially dangerous for a large prism because stack space is limited. In particular, it could crash for prisms more than about 100,000 elements. |
|
For the prism list, in practice the
|
|
I implemented my proposal and pushed a commit to this PR. |
|
The latest changes (c617512) are producing failures for the Meep unit test
|
intersect_line_segment_with_mesh added (b - last_s) when a hit fell at or past b, then the post-loop cleanup added the same length again because inside/last_s were not updated in the break branch. Drop the in-branch addition and let the post-loop check handle the inside-at-b case. Add test_cube_segments_vs_block: 1000 random unit-direction line segments through a unit cube mesh vs make_block, comparing interior lengths within 1e-4. This test surfaced the bug; max diff after the fix is ~4e-16. Also note that remove_duplicate_intersections should be unified with the prism slist dedup after NanoComp#75.
Root cause: Two thread-safety bugs in
utils/geom.c, exposed by Meep's recently added OMP parallization ofset_chi1inv(NanoComp/meep#3166).Fix 1 (primary — caused the crash of
test_mode_coeffs.py): Remove thegeom_fix_object_ptr(&o)call fromgeom_get_bounding_box(). For Prism objects, this call triggeredreinit_prism(), which frees and re-mallocs all internal prism arrays. When multiple OMP threads call this concurrently on the same Prism, it causes double-free/use-after-free, corrupting heap metadata and producing thefree(): unaligned chunk detected in tcache 2error.Fix 2 (secondary — would cause incorrect results): Change
intersect_line_segment_with_prism()to use a stack-allocated variable-length array (VLA) (double slist[num_vertices + 2]) instead of the sharedprsm->workspace.itemsbuffer. This prevents data races during concurrent intersection calculations.Verification: all 6 tests in Meep's
test_mode_coeffs.pynow pass withOMP_NUM_THREADS=1,2, and4.@Luochenghuang